Code Review

Solid implementation of the menubar support infrastructure. The auto-port allocation and server registry are well-designed. I've identified a few bugs and potential improvements below.

Critical Issues

1. Race Condition in Port Allocation (src/commands/tdd-daemon.js:67-81)

Issue: There's a TOCTOU (time-of-check-time-of-use) race condition between finding an available port and actually starting the server.

// Auto-allocate an available port
port = await registry.findAvailablePort();
autoAllocated = port !== 47392;

// ... later, server starts on this port
// But another process could grab it in between!

If two vizzly tdd start commands run simultaneously without --port, they could both get assigned the same port from findAvailablePort(), then both try to bind to it, causing one to fail with an unclear error.

Fix: Consider one of these approaches:

• Reserve the port immediately in the registry before starting the daemon (optimistic locking)
• Retry with a new port if the daemon fails to start due to EADDRINUSE
• Use a lock file during port allocation

2. Top-level Await Without Error Handling (src/cli.js:758-759)

Issue: The saveUserPath() call uses top-level await with a catch-all that silently swallows all errors:

// Save user's PATH for menubar app before parsing commands
// This auto-configures the menubar app so it can find npx/node
await saveUserPath().catch(() => {});

If saveUserPath() hangs or takes a long time (e.g., due to filesystem issues), it will block the entire CLI startup, making all commands slow. Also, the empty catch handler makes debugging issues impossible.

Fix:

• Remove await - this doesn't need to block CLI startup
• Add timeout protection
• Log errors in verbose mode

saveUserPath()
  .catch(err => {
    // Only log in verbose mode to avoid noise
    if (program.opts().verbose) {
      output.debug('cli', 'Failed to save PATH for menubar:', err.message);
    }
  });

Bugs

3. Incorrect osascript Syntax (src/tdd/server-registry.js:166)

Issue: The osascript command for posting notifications is incorrect:

execSync(
  `osascript -e 'tell application "System Events" to post notification with name "dev.vizzly.serverChanged"'`,
  { stdio: 'ignore', timeout: 1000 }
);

This won't work - System Events doesn't have a post notification command with that syntax. You probably want to use NSDistributedNotificationCenter via a different mechanism.

Fix: Either:

• Use osascript to run Swift/ObjC code that posts the notification correctly
• Use a small helper binary that the menubar app installs
• Have the menubar app poll servers.json with file watching (simpler and more reliable)

Since the menubar app needs to watch servers.json anyway for live updates, polling might be simpler than trying to get notifications working cross-process.

4. Registry Cleanup Logic Inconsistency (src/commands/tdd-daemon.js:53-54)

Issue: When a stale server is detected, it's removed from the registry but the local .vizzly/server.json might still exist, causing confusion:

} else {
  // Stale entry - clean it up
  registry.unregister({ directory: process.cwd() });
}

The local server.json file could still be there, making SDK discovery behave unexpectedly.

Fix: Also clean up the local files when detecting stale entries:

} else {
  // Stale entry - clean it up
  registry.unregister({ directory: process.cwd() });
  
  const vizzlyDir = join(process.cwd(), '.vizzly');
  const pidFile = join(vizzlyDir, 'server.pid');
  const serverFile = join(vizzlyDir, 'server.json');
  if (existsSync(pidFile)) unlinkSync(pidFile);
  if (existsSync(serverFile)) unlinkSync(serverFile);
}

Code Quality

5. Missing Input Validation (src/tdd/server-registry.js:57-80)

Issue: The register() method doesn't validate required fields, which could lead to corrupt registry entries:

register(serverInfo) {
  // No validation that pid, port, directory are provided
  registry.servers.push({
    id: serverInfo.id || randomBytes(8).toString('hex'),
    port: Number(serverInfo.port),  // Number(undefined) = NaN
    pid: Number(serverInfo.pid),    // Number(undefined) = NaN
    // ...
  });
}

Fix: Add validation:

register(serverInfo) {
  if (!serverInfo.pid || !serverInfo.port || !serverInfo.directory) {
    throw new Error('Missing required fields: pid, port, directory');
  }
  
  const port = Number(serverInfo.port);
  const pid = Number(serverInfo.pid);
  
  if (isNaN(port) || isNaN(pid)) {
    throw new Error('Invalid port or pid - must be numbers');
  }
  // ...
}

6. Unregister Should Match Both Port AND Directory (src/tdd/server-registry.js:86-97)

Issue: The current logic uses OR when both are provided:

unregister({ port, directory }) {
  if (port) {
    registry.servers = registry.servers.filter(s => s.port !== port);
  }
  if (directory) {
    registry.servers = registry.servers.filter(
      s => s.directory !== directory
    );
  }
}

If you call unregister({ port: 47392, directory: '/foo' }), it removes all servers on port 47392 AND all servers in /foo, which might not be what you want.

Fix: Use AND logic when both are provided:

unregister({ port, directory }) {
  if (port && directory) {
    // Both specified - match both
    registry.servers = registry.servers.filter(
      s => !(s.port === port && s.directory === directory)
    );
  } else if (port) {
    registry.servers = registry.servers.filter(s => s.port !== port);
  } else if (directory) {
    registry.servers = registry.servers.filter(s => s.directory !== directory);
  }
}

7. Hardcoded Console.warn (src/tdd/server-registry.js:41)

Issue: Uses console.warn instead of the output utility:

console.warn('Warning: Could not read server registry, starting fresh');

This bypasses the CLI's output configuration (JSON mode, color settings, etc.).

Fix: Use the output module (you'll need to import it):

import * as output from '../utils/output.js';

// In read():
} catch (_err) {
  output.warn('Could not read server registry, starting fresh');
}

8. Platform-specific Code Without Tests (src/tdd/server-registry.js:160-172)

Issue: The notifyMenubar() method is macOS-specific but has no indication this is tested on macOS. The osascript command might fail silently.

Fix: Add a comment indicating this is untested or needs macOS verification:

/**
 * Notify the menubar app that the registry changed
 * Uses macOS DistributedNotificationCenter via osascript
 * 
 * NOTE: This implementation is experimental and may not work as expected.
 * Consider file watching as a more reliable alternative.
 */

Performance Considerations

9. Synchronous File I/O in Request Path (src/tdd/server-registry.js)

Issue: All registry operations use synchronous file I/O (readFileSync, writeFileSync). While this isn't on a hot path, it could cause CLI responsiveness issues if the filesystem is slow.

Recommendation: Consider async I/O for better responsiveness, especially since the daemon startup is already async. Not critical for this PR, but worth noting for future optimization.

Test Coverage

Excellent Test Coverage ✅

The test suite in tests/tdd/server-registry.test.js is comprehensive:

• All major code paths covered (register, unregister, find, list, cleanup)
• Edge cases tested (corrupted JSON, stale PIDs, port conflicts)
• Actual port occupation testing with TCP servers
• 17 tests covering the core functionality

Minor gap: No tests for the macOS notification code (understandable since it's platform-specific).

Security Considerations

10. Directory Traversal Protection

The registry stores absolute paths in directory fields. Make sure there's validation elsewhere in the codebase that prevents directory traversal attacks if user input ever flows into these paths. From this PR, it looks like only process.cwd() is used, which is safe.

Summary

Severity Breakdown:

• 🔴 Critical: 2 (port race condition, blocking startup)
• 🟡 Bugs: 2 (osascript syntax, stale cleanup)
• 🔵 Code Quality: 6 (validation, error handling, consistency)

Strengths:

• Clean architecture with good separation of concerns
• Excellent test coverage for core functionality
• Good error recovery (corrupted JSON, stale processes)
• Auto-port allocation UX is intuitive

Recommendations:

1. Fix the port allocation race condition before merging (critical)
2. Make saveUserPath() non-blocking to avoid CLI startup delays
3. Consider file watching instead of osascript notifications for reliability
4. Add input validation to prevent corrupt registry entries

Great work on the auto-port allocation feature - this will be a huge UX improvement for running multiple projects simultaneously!

Vizzly - Visual Test Results

_{feat/menubar-support · 76f7649a}